Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

update to CRAN version 1.0.7 #340

Merged
merged 11 commits into from
Mar 22, 2021
Merged

update to CRAN version 1.0.7 #340

merged 11 commits into from
Mar 22, 2021

Conversation

mlampros
Copy link
Collaborator

This pull request is for the update of the RGF R package to CRAN version 1.0.7. As discussed in the comment of the pull request 338 I've modified the code because there were 2 NOTES for the Windows version that had to be fixed.
As for the additional issue of the new M1mac chip-tests that was discussed in the comment of the pull request 338 we have to keep an eye on the check results page of the R package.

@mlampros mlampros requested review from fukatani and StrikerRUS March 17, 2021 18:43
@mlampros
Copy link
Collaborator Author

mlampros commented Mar 17, 2021 •
edited
Loading

It gives an error or R ubuntu-latest

* checking PDF version of manual ... WARNING
LaTeX errors when creating PDF version.
This typically indicates Rd problems.
LaTeX errors found:
Undefined control sequence.
....

I re-run the tests but it persists. I don't think that it's related with the code.

In the second run of the tests it gives a NOTE on mac OS because one example ran for more than 5 seconds.

Is there any way to re-run only the R tests? It seems that the Re-run jobs button applies to all tests (both R and Python)

@StrikerRUS
Copy link
Member

@mlampros Thank you very much for your hard work!

I re-run the tests but it persists. I don't think that it's related with the code.

We are seeing some LaTeX-related errors in LightGBM right now: microsoft/LightGBM#3997 (comment) too. Let's re-run tests tomorrow and see whether we need to dig deeper into the problem or it is temporary just due to some servers' problems.

Is there any way to re-run only the R tests?

Unfortunately, it is impossible in GitHub Actions to re-run a separate job.

Only one frustrating thing that I noticed during more than 6 months of usage is that you cannot re-run a separate job, you should rerun all of them.
#328 (comment)

@StrikerRUS
Copy link
Member

StrikerRUS commented Mar 17, 2021 •
edited
Loading

In the second run of the tests it gives a NOTE on mac OS because one example ran for more than 5 seconds.

Please increase the allowed time to run by overwriting _R_CHECK_EXAMPLE_TIMING_THRESHOLD_ environment variable. Refer to microsoft/LightGBM#4049 (comment) and https://github.com/microsoft/LightGBM/pull/4055/files.

@mlampros
Copy link
Collaborator Author

mlampros commented Mar 18, 2021 •
edited
Loading

I am out of ideas for this error case on ubuntu-latest. I tried various modifications in the 'r_tests.sh' file but none of these work.

As a reference I'll add the following links that I stumbled upon during my search:

@StrikerRUS StrikerRUS mentioned this pull request Mar 18, 2021
Closed
@StrikerRUS
Copy link
Member

@mlampros
I've just re-run all CI jobs in this PR and at the same time created a dummy PR from the master branch. That PR has been built successfully. So now we can be sure that the reason of failing CI is in the changes from this PR. I'll try to investigate breaking changes this weekend.

@mlampros
Copy link
Collaborator Author

mlampros commented Mar 18, 2021 •
edited
Loading

So, you mean that my changes in the examples section of the package is the main reason for the error (actually the .Rd documentation error). But this error would have appeared also during the submission to CRAN, which is not the case.

@StrikerRUS
Copy link
Member

StrikerRUS commented Mar 19, 2021 •
edited
Loading

So, you mean that my changes in the examples section of the package is the main reason for the error (actually the .Rd documentation error).

Yeah, it seems to be so.

But this error would have appeared also during the submission to CRAN, which is not the case.

I guess the difference in the environments of our CI and CRAN (software versions or incomplete installation, for example) led to such results.

@StrikerRUS
Copy link
Member

@mlampros
Hey!

I'll try to investigate breaking changes this weekend.

Please merge #342 into this PR to make it pass all CI checks. I left some comments there explaining fixes.

@StrikerRUS
fix some errors in R-package after upgrade to 1.0.7 (#342)
2651369 
* fixes

* test

* test

* fixes

* fixes

* fixes

* fixes
@StrikerRUS
Copy link
Member

@mlampros
Seems that I've managed to make our CI checks compatible with roxygen2 > 7.0. Please take a look: #343. Sorry for the inconvenience!

StrikerRUS
StrikerRUS approved these changes Mar 21, 2021
View reviewed changes
Copy link
Member

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the great job!
Cool that we don't need to regenerate docs with outdated roxygen2 version and docs are in consistency with ones on CRAN. I agree that rewriting them to fix warnings is time consuming and shouldn't have a high priority.
I left only 3 very minor code style suggestions which you can accept directly in your browser if you agree with them.

.ci/r_tests_windows.ps1 Outdated Show resolved Hide resolved
.ci/r_tests_windows.ps1 Outdated Show resolved Hide resolved
R-package/tests/testthat/helper-skip.R Outdated Show resolved Hide resolved
@StrikerRUS StrikerRUS merged commit effad8b into master Mar 22, 2021
@StrikerRUS StrikerRUS deleted the R-package-update-cran branch March 22, 2021 10:55
@StrikerRUS StrikerRUS mentioned this pull request Mar 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StrikerRUS StrikerRUS StrikerRUS approved these changes

@fukatani fukatani Awaiting requested review from fukatani

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mlampros @StrikerRUS